Add ns :require sorting support #113
Conversation
|
Thanks for the PR. I'll have time for a more thorough review later, but my initial thoughts is that your function is too large, and you're not making use of existing functions so you're duplicating functionality. You're also using your comments to explain what rather than why; if you're doing that, it's a strong indication that you need to split up your function instead. For example, you've written: ; check if this is a require node
(if-let [req (z/find
(z/right node)
z/next
(fn [[{:keys [children]}]]
(= :require (-> children first :k))))]But a better way would be to write: (if (require-node? node)See how it conveys the same information, but without the need for a comment? Comments should be used sparingly, to explain why in the cases where it's not obvious. If the what isn't obvious, then more functions are required. However, I don't think you need to use a loop at all. Instead, use (defn sort-ns-requires [form]
(transform form edit-all ns-require? sort-namespaces))This will traverse the form, looking for a zipper loc that adheres to I'll be able to give you more detailed suggestions later on. |
|
Thanks @weavejester for the suggestions - I missed some useful utility functions! I've made some the of the changes you suggest, though I think the |
cljfmt/src/cljfmt/core.cljc
Outdated
| (recur (rest unsorted-requires) sorted-requires (conj new-children (first unsorted-requires))))))))))) | ||
|
|
||
| (defn require-node? [zloc] | ||
| (= :require (z/sexpr (z/down zloc)))) |
There was a problem hiding this comment.
You need to ensure that the :require is within a top-level ns declaration.
There was a problem hiding this comment.
Good catch. I'll add another test for that.
|
|
||
| (defn sort-requires | ||
| [require-zloc] | ||
| (z/replace |
There was a problem hiding this comment.
Can you explain what you're doing with this function? Also how does it handle changing the indent level?
There was a problem hiding this comment.
This bit is a little tricky. First it collect all the vectors within the (:require..), sorts them, then effectively plays them back in sorted order in the loop, replacing only the vector nodes. This leaves all the original whitepspace nodes in place.
There was a problem hiding this comment.
That's an interesting approach. It would mean that comments wouldn't follow their associated clause, but comments are rare in the ns declaration anyway.
You seem to be looking for vectors, but namespaces in require can just be symbols. Perhaps instead look for non-whitespace tokens?
Maybe you want something more like:
(let [nodes (partition-by whitespace? children)
spaces (take-nth 2 nodes)
forms (take-nth 2 (rest nodes))]
(->> forms (sort-by sexpr) (interleave spaces) (apply concat)))Or even factor out the unravelling:
(defn unravel [n coll]
(map #(take-nth n (drop % coll)) (range n)))
;; ...
(let [[spaces forms] (->> children (partition-by whitespace?) (unravel 2))]
(->> forms (sort-by sexpr) (interleave spaces) (apply concat)))Obviously that's pseudo-ish code, but should give you a rough idea of what I mean.
There was a problem hiding this comment.
I think it'd be cool if it worked with comments too, so I'll take a look at that. We can't rely on the same hard-coded unraveling then though.
|
Closed by 23daaf0 |
:sort-ns-requires?which defaults tofalsefor now:requirevectors (ignores symbols, lists etc)